-
Notifications
You must be signed in to change notification settings - Fork 6.3k
8367319: Add os interfaces to get machine and container values separately #27646
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
8367319: Add os interfaces to get machine and container values separately #27646
Conversation
👋 Welcome back cnorrbin! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
@caspernorrbin The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
Then, is it possible to keep the original API for "most use cases"? For others, they can query The current proposed API kind of forces all users to distinguish btw inside a container or not, even though most use cases don't care. |
The original API is untouched, so "most use cases" can still use that without ever worrying about containers. The |
I see; I misunderstood the proposal. Then, there are 3 sets of public APIs, generic, machine_, and container_. I'd expect most use the generic ones and some might query the machine_ ones, so I don't see much need for container_ ones to be public APIs. YMMV. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will conflict mightily with the refactoring that I'm working on for https://bugs.openjdk.org/browse/JDK-8365606
} | ||
|
||
double os::container_processor_count() { | ||
assert(is_containerized(), "must be running containerized"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the equivalent of assert(false)
. Shouldn't this be ShouldNotReachHere()
?
That is a mis-characterization of the API. (**) "physical" actually has no meaning these days. There is some value you can obtain through the operating system that provides the maximum number of processors that the operating system can see (and thus make available to the JVM). |
What is a "machine" here? Historically we have misused "physical" to mean what does a bare-metal OS report on a bare-metal piece of hardware. But that became inaccurate decades ago once virtualization/hypervisors arrived. So we've adjusted API's (e.g. MXBeans) to report whatever the "operating system" reports. The problem there is some things the operating system reports take into account the presence of containers, and others do not. This has always been a problem with these container environments - they should be invisible to software but they are not.
For a long time this was an impossible question to answer accurately - we could query whether cgroups were configured on a system but we couldn't ask if the JVM process was running under any cgroup constraints - has that changed? I would like to get a better idea of what kinds of "machine" information we need to query and how it will be used. I mean, how does it help to know a "machine" has 256 processors if the various software layers only make 16 available to you? |
Agreed, I conflated the two here. What I actually should have written is like you said, the number of logical processors available for the JVM to execute on. That is also the value the new By contrast, the current container-reported value treats cpu quota and logical processors as the same thing, even though quota only restricts cpu time, not the number of cores we can run on. With a quota of 1, we might still execute on two cores for 50% of the time each, but
Machine in this context is the values the operating system reports, which could already be limited depending on the configuration. All this is of course Linux-only, as we don't support containers on any other platforms. In many container deployments the cgroup limits do differ from the OS view, but in a fully virtualized environment they can coincide. In that situation none of this makes a difference anyways, and both functions would report the same value.
Our container detection still isn't perfect, but it has improved:
These heuristics can miss more exotic setups but are pretty accurate for most use cases today.
Like I mentioned above when explaining The same argument applies to memory. GC heuristics may want to look at overall memory pressure on the machine, not just the container's limit. Imagine several containers on one host, where one is consuming most of its limit while others are relatively idle. From the host perspective the system is still comfortable, so the GC could be less aggressive compared to if every container was close to its limit. Without access to the host-level numbers we can't distinguish these scenarios. Admittedly these functions are niche and will only matter for very specialised, performance-critical tasks. Still, the information is already available from the operating system, and the JVM should not hide or overwrite it for those users who can benefit from it. |
Understood, thanks for flagging that. The amount of change in the container layer isn't that big except for |
Right - the way the "container folk" dealt with quotas etc never made any sense to me at all. I tried arguing the point but to no avail. So basically what you are looking for here is a way to get around the "broken" definition of available-processors when quotas are enforced. |
Partly, yes. The cpu quota is the clearest example, but the same mismatch shows up for other values. The meaning of the container values don't always map 1:1 to OS numbers. What I'm after is a clean way to get those numbers separately. Pairing |
Separating out the |
Hi everyone,
The current
os::
layer on Linux hides whether the JVM is running inside a container or not. When running inside a container, we replace machine values with container values where applicable, without telling the user of these methods. For most use cases, this is fine, users only care about the returned value. But for other use cases, where the value originated is important. Two examples:os::active_processor_count()
only returns the limited container value, which also represents something slightly different.os::physical_memory()
only gives one number.To solve this, every function that mixed container/machine values now has to explicit variants, prefixed with
machine_
andcontainer_
. These use the bool return + out-parameter interface, with the container functions only working on Linux. The original methods remain and continue to return the same mixed values.In addition, container-specific accessors for the memory soft limit and the memory throttle limit have been added, as these values matter when running in a containerized environment.
OSContainer::active_processor_count()
has also been changed to returndouble
instead ofint
. The previous implementation rounded the quota/period ratio up to produce an integer foros::active_processor_count()
. Now, when the value is requested directly from the new container API it makes more sense to preserve this fraction rather than rounding it up. We can thus keep the exact value for those that want it, then round it up to keep the same behavior inos::active_processor_count()
.Testing:
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27646/head:pull/27646
$ git checkout pull/27646
Update a local copy of the PR:
$ git checkout pull/27646
$ git pull https://git.openjdk.org/jdk.git pull/27646/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27646
View PR using the GUI difftool:
$ git pr show -t 27646
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27646.diff
Using Webrev
Link to Webrev Comment